feat: configurable auto-zoom settings with UI sliders#1663
feat: configurable auto-zoom settings with UI sliders#1663namearth5005 wants to merge 6 commits intoCapSoftware:mainfrom
Conversation
…nfigurable fields
…placing hardcoded constants
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| <SettingSlider | ||
| label="Sensitivity" | ||
| value={ | ||
| settings.autoZoomConfig?.movementWindowDistanceThreshold ?? | ||
| 0.08 | ||
| } | ||
| onChange={(v) => | ||
| handleConfigChange("movementWindowDistanceThreshold", v) | ||
| } | ||
| min={0.02} | ||
| max={0.2} | ||
| step={0.01} | ||
| format={(v) => { | ||
| if (v <= 0.05) return "High"; | ||
| if (v <= 0.12) return "Medium"; | ||
| return "Low"; | ||
| }} | ||
| /> |
There was a problem hiding this comment.
The "Sensitivity" slider adjusts movementWindowDistanceThreshold (cumulative window distance) but leaves movementEventDistanceThreshold (per-frame distance) fixed at 0.02. In generate_zoom_segments_from_clicks_impl, movement detection is an OR condition:
let significant_movement = distance >= movement_event_distance_threshold // fixed: 0.02
|| window_distance >= movement_window_distance_threshold; // slider-controlledThis means even at "Low" sensitivity (movementWindowDistanceThreshold = 0.2), any single frame where the cursor travels ≥ 2% of normalised screen width still triggers a zoom. In practice the per-frame branch can dominate, making the "Low" end of the slider much weaker than users expect.
To make the slider fully effective, both thresholds should scale together. You could either:
- Expose
movementEventDistanceThresholdas part of the sensitivity curve, or - Derive
movementEventDistanceThresholdproportionally from themovementWindowDistanceThresholdvalue rather than keeping it at its hardcoded default.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/routes/(window-chrome)/settings/experimental.tsx
Line: 170-187
Comment:
**"Sensitivity" slider only controls half the detection condition**
The "Sensitivity" slider adjusts `movementWindowDistanceThreshold` (cumulative window distance) but leaves `movementEventDistanceThreshold` (per-frame distance) fixed at `0.02`. In `generate_zoom_segments_from_clicks_impl`, movement detection is an **OR** condition:
```rust
let significant_movement = distance >= movement_event_distance_threshold // fixed: 0.02
|| window_distance >= movement_window_distance_threshold; // slider-controlled
```
This means even at "Low" sensitivity (`movementWindowDistanceThreshold = 0.2`), any single frame where the cursor travels ≥ 2% of normalised screen width still triggers a zoom. In practice the per-frame branch can dominate, making the "Low" end of the slider much weaker than users expect.
To make the slider fully effective, both thresholds should scale together. You could either:
- Expose `movementEventDistanceThreshold` as part of the sensitivity curve, or
- Derive `movementEventDistanceThreshold` proportionally from the `movementWindowDistanceThreshold` value rather than keeping it at its hardcoded default.
How can I resolve this? If you propose a fix, please make it concise.| let settings = GeneralSettingsStore::get(&app) | ||
| .unwrap_or(None) | ||
| .unwrap_or_default(); |
There was a problem hiding this comment.
ok().flatten()
unwrap_or(None) on a Result<Option<T>> is non-idiomatic and silently discards the error. The codebase already uses a cleaner pattern elsewhere (e.g. recording.rs:598 uses .ok().flatten()):
| let settings = GeneralSettingsStore::get(&app) | |
| .unwrap_or(None) | |
| .unwrap_or_default(); | |
| let settings = GeneralSettingsStore::get(&app) | |
| .ok() | |
| .flatten() | |
| .unwrap_or_default(); |
This makes the intent explicit — convert Err to None, then fall back to defaults — and is consistent with the rest of the file.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src-tauri/src/lib.rs
Line: 2119-2121
Comment:
**Idiomatic error handling with `ok().flatten()`**
`unwrap_or(None)` on a `Result<Option<T>>` is non-idiomatic and silently discards the error. The codebase already uses a cleaner pattern elsewhere (e.g. `recording.rs:598` uses `.ok().flatten()`):
```suggestion
let settings = GeneralSettingsStore::get(&app)
.ok()
.flatten()
.unwrap_or_default();
```
This makes the intent explicit — convert `Err` to `None`, then fall back to defaults — and is consistent with the rest of the file.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
1 participant
Summary
Promotes auto-zoom from experimental toggle to configurable feature (refs #1646):
AutoZoomConfigstruct — extracts all 11 hardcoded constants from zoom segment generation into a serializable, backward-compatible config with#[serde(default)]GeneralSettingsStore→ Tauri IPC command →generate_zoom_segments_from_clicks_implFiles changed
crates/project/src/configuration.rsAutoZoomConfigstruct with 11 fieldsapps/desktop/src-tauri/src/general_settings.rsauto_zoom_configtoGeneralSettingsStoreapps/desktop/src-tauri/src/recording.rsapps/desktop/src-tauri/src/lib.rsapps/desktop/src/routes/.../experimental.tsxSettingSlidercomponent and 3 config slidersBackward compatibility
Every new field uses
#[serde(default)]with values matching the old hardcoded constants:AUTO_ZOOM_AMOUNTCLICK_GROUP_TIME_THRESHOLD_SECSCLICK_GROUP_SPATIAL_THRESHOLDCLICK_PRE_PADDINGCLICK_POST_PADDINGMERGE_GAP_THRESHOLDMIN_SEGMENT_DURATIONThis is the first of 5 incremental PRs to promote auto-zoom to production quality.
Test plan
AutoZoomConfig::default()— verified locally (cargo check -p cap-projectpasses; tests require ffmpeg for fullcap-desktopbuild)Greptile Summary
This PR promotes auto-zoom from an on/off experimental toggle to a fully configurable feature by extracting 11 hardcoded constants into a new
AutoZoomConfigstruct that is persisted inGeneralSettingsStoreand surfaced via three UI sliders (Zoom Amount, Sensitivity, Smoothing). The Rust side is clean and backward-compatible — all defaults match the previous constants and#[serde(default)]is used throughout. The TypeScript UI is well-structured with a reusableSettingSlidercomponent.Notable issues:
movementEventDistanceThreshold(per-frame) is left at its default0.02while onlymovementWindowDistanceThresholdis exposed. Since the significance check uses||, the per-frame branch can still fire at the "Low" end of the slider, making the sensitivity control weaker than users would expect.Inner(lines 68–80) mirrorsAutoZoomConfig::default()exactly, creating a hidden maintenance dependency that could silently drift.unwrap_or(None)on the settings fetch inlib.rsis functional but less readable than the.ok().flatten()pattern already used elsewhere in the codebase.Confidence Score: 3/5
apps/desktop/src/routes/(window-chrome)/settings/experimental.tsx— the Sensitivity slider and the hardcoded default config object both warrant a second look before this exits experimental.Important Files Changed
Sequence Diagram
sequenceDiagram participant UI as experimental.tsx participant Store as generalSettingsStore participant IPC as Tauri IPC participant GSS as GeneralSettingsStore participant REC as recording.rs participant CFG as AutoZoomConfig UI->>Store: generalSettingsStore.set({ autoZoomConfig }) Store->>IPC: persists to disk (JSON with camelCase) Note over UI,IPC: Editor — "Generate zoom from clicks" UI->>IPC: generate_zoom_segments_from_clicks() IPC->>GSS: GeneralSettingsStore::get(&app) GSS-->>IPC: Ok(Some(settings)) or fallback to Default IPC->>REC: generate_zoom_segments_for_project(meta, recordings, &settings.auto_zoom_config) REC->>CFG: destructure AutoZoomConfig fields CFG-->>REC: zoom_amount, thresholds, paddings… REC-->>IPC: Vec<ZoomSegment> IPC-->>UI: zoom segments applied to timeline Note over REC,CFG: Recording completion (studio mode) REC->>GSS: GeneralSettingsStore::get(&app) GSS-->>REC: settings with auto_zoom_config REC->>REC: generate_zoom_segments_from_clicks(recording, recordings, &settings.auto_zoom_config) REC-->>REC: project_config with zoom_segmentsComments Outside Diff (1)
apps/desktop/src/routes/(window-chrome)/settings/experimental.tsx, line 58-82 (link)The
autoZoomConfigdefault object on lines 68–80 exactly mirrors the values inAutoZoomConfig::default()in Rust. This creates a silent maintenance hazard: if any default changes on the Rust side (e.g. tuningclick_post_paddingin a later PR), the TypeScript fallback silently stays stale and the UI will show incorrect defaults wheninitialStoreisnull.Since
initialStorecomes fromgeneralSettingsStore.get()and the backend already applies#[serde(default)], the TypeScript fallback is only exercised before the first async load. Consider trusting the backend for defaults and initialising the store without an inlineautoZoomConfigoverride, or at minimum adding a comment linking the two so reviewers know to keep them in sync.Prompt To Fix With AI
Prompt To Fix All With AI
Last reviewed commit: "style: apply cargo f..."
(2/5) Greptile learns from your feedback when you react with thumbs up/down!